Skip to content

Conversation

@janvorli
Copy link
Member

This makes around 20 more coreclr tests pass with the interpreter.

This makes around 20 more coreclr tests pass with the interpreter.
@janvorli janvorli added this to the 11.0.0 milestone Oct 28, 2025
@janvorli janvorli self-assigned this Oct 28, 2025
@janvorli janvorli requested a review from BrzVlad as a code owner October 28, 2025 00:10
Copilot AI review requested due to automatic review settings October 28, 2025 00:10
@janvorli janvorli requested a review from kg as a code owner October 28, 2025 00:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements support for the CEE_JMP instruction in the CoreCLR interpreter, enabling approximately 20 additional coreclr tests to pass. The JMP instruction performs a tail call by transferring control to another method with the current method's arguments, without creating a new stack frame.

Key changes:

  • Added INTOP_JMP opcode definition for interpreter operations
  • Implemented CEE_JMP compilation in the interpreter compiler
  • Added JMP execution logic that reuses the current frame for the target method

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/coreclr/interpreter/inc/intops.def Defines the new INTOP_JMP opcode with method handle operand
src/coreclr/interpreter/compiler.cpp Implements compilation of CEE_JMP IL instruction to INTOP_JMP
src/coreclr/vm/interpexec.cpp Adds execution logic for INTOP_JMP, including frame reinitialization and assertion for managed-only targets

@davidwrighton
Copy link
Member

I would prefer to have written this on top of our tail-call logic (even though it would be slower), so as to avoid adding more complexity to the interpreter execution logic for a scenario which is vanishingly rare.

@janvorli
Copy link
Member Author

There is still something not right, based on debugging some other test that accidentally also uses CEE_JMP. I am setting no-merge label until I sort it out.

@janvorli janvorli added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 28, 2025
@janvorli janvorli removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 28, 2025
@janvorli
Copy link
Member Author

Problem fixed

@am11
Copy link
Member

am11 commented Oct 28, 2025

This makes around 20 more coreclr tests pass with the interpreter.

@janvorli, is there a tracking issue of remaining instructions? The list at #112158 seems like it was completed but not updated.

@kg
Copy link
Member

kg commented Oct 28, 2025

Can the JMP target be a generic method? If so, do we need to do something we aren't doing here? I don't see any logic that would handle it, but maybe it would just automatically work?

@davidwrighton
Copy link
Member

@kg In principle, yes. And this implementation wouldn't work for that. I would be ok with making jmp to a generic method or static method on a generic type cause a BADCODE, since I don't think we have evidence that anyone has ever used jmp with a generics. Looking at the implementation of JMP in the jit, I'm not confident it handles that situation correctly either. This is another reason, I would prefer an implementation that used the general purpose tail-call logic we've built, as that is something we have real testing for.

@jakobbotsch
Copy link
Member

jakobbotsch commented Oct 29, 2025

Looking at the implementation of JMP in the jit, I'm not confident it handles that situation correctly either

I think for cross-genericness jumps using generic context the JIT will badcode too here:

eeGetMethodSig(resolvedToken.hMethod, &sig);
if (sig.numArgs != info.compMethodInfo->args.numArgs ||
sig.retType != info.compMethodInfo->args.retType ||
sig.callConv != info.compMethodInfo->args.callConv)
{
BADCODE("Incompatible target for CEE_JMPs");
}

It might work if both the caller and callee use a generic context parameter.

As an aside, I would echo what @davidwrighton suggests to represent this via tailcalls, if possible. In the JIT we have a similar explicit representation for CEE_JMP and it has caused a number of issues over the years. One of the major ones is that the JIT's internal representation of these jumps does not list the arguments being passed as operands explicitly, so it is not immediately obvious to transformations that there is a use of some values when it sees the internal jump operation.

Of course I don't know if the interpreter would be inclined to ever run into similar issues -- they have been mostly in optimized code.

@janvorli janvorli mentioned this pull request Oct 29, 2025
66 tasks
@janvorli
Copy link
Member Author

@janvorli, is there a tracking issue of remaining instructions? The list at #112158 seems like it was completed but not updated.

@am11 the CEE_JMP is the last one that was not implemented yet. We've forgotten to update #112158 w.r.t. to the other implemented ones. I've just updated it.

@janvorli
Copy link
Member Author

This makes around 20 more coreclr tests pass with the interpreter.

It is actually 55, not 20. The 20 was with an issue I had in the initial state of the PR and fixed later.

@janvorli
Copy link
Member Author

Based on the comments from @davidwrighton and @jakobbotsch, I am working on moving the implementation to what @davidwrighton suggested.

@MichalPetryka
Copy link
Contributor

Looking at the implementation of JMP in the jit, I'm not confident it handles that situation correctly either

Would it make sense to add a test here that ensures both the JIT and Interpreter behave the same?

@janvorli
Copy link
Member Author

I've pushed another commit that changes the CEE_JMP handling based on @davidwrighton and @jakobbotsch comments. It is now implemented purely at the compilation side. It loads the current method's args and then emulates the CEE_JMP using a tail call.

@am11
Copy link
Member

am11 commented Oct 30, 2025

Looking at the implementation of JMP in the jit, I'm not confident it handles that situation correctly either

Would it make sense to add a test here that ensures both the JIT and Interpreter behave the same?

AI generated these four test cases.
⚙️ Test 1 — Jump from one regular method to another

```msil
.assembly JmpTest1 {}
.assembly extern mscorlib {}

.class public auto ansi beforefieldinit Program extends [mscorlib]System.Object
{
    .method public static void Target() cil managed
    {
        .maxstack 8
        ldstr "Target called"
        call void [mscorlib]System.Console::WriteLine(string)
        ret
    }

    .method public static void Source() cil managed
    {
        jmp void Program::Target()
    }

    .method public static void Main() cil managed
    {
        call void Program::Source()
        ret
    }
}
```

🟢 Expected behavior:
```
Target called
```

⚙️ Test 2 — Jump from generic method to generic method

```msil
.assembly JmpTest2 {}
.assembly extern mscorlib {}

.class public auto ansi beforefieldinit Program extends [mscorlib]System.Object
{
    .method public static void Target<T>() cil managed
    {
        .maxstack 8
        ldstr "Target<T> called"
        call void [mscorlib]System.Console::WriteLine(string)
        ret
    }

    .method public static void Source<T>() cil managed
    {
        jmp void Program::Target<!T>()
    }

    .method public static void Main() cil managed
    {
        call void Program::Source<int32>()
        ret
    }
}
```

🟢 Expected:
```
Target<T> called
```

⚙️ Test 3 — Jump from generic method → regular method
```msil
.assembly JmpTest3 {}
.assembly extern mscorlib {}

.class public auto ansi beforefieldinit Program extends [mscorlib]System.Object
{
    .method public static void Target() cil managed
    {
        .maxstack 8
        ldstr "Regular Target called"
        call void [mscorlib]System.Console::WriteLine(string)
        ret
    }

    .method public static void Source<T>() cil managed
    {
        jmp void Program::Target()
    }

    .method public static void Main() cil managed
    {
        call void Program::Source<int32>()
        ret
    }
}
```

🟢 Expected:

```
Regular Target called
```

⚙️ Test 4 — Jump from regular method → generic method
```msil
.assembly JmpTest4 {}
.assembly extern mscorlib {}

.class public auto ansi beforefieldinit Program extends [mscorlib]System.Object
{
    .method public static void Target<T>() cil managed
    {
        .maxstack 8
        ldstr "Generic Target<T> called"
        call void [mscorlib]System.Console::WriteLine(string)
        ret
    }

    .method public static void Source() cil managed
    {
        jmp void Program::Target<int32>()
    }

    .method public static void Main() cil managed
    {
        call void Program::Source()
        ret
    }
}
```

🟢 Expected:
```
Generic Target<T> called
```

@janvorli
Copy link
Member Author

AI generated these four test cases.

Thank you @am11! I am going to try these tests and add them to this PR

* Make the handling of generic methods with the CEE_JMP equivalent to JIT
* Add tests verifying that
@janvorli
Copy link
Member Author

@MichalPetryka I've added the tests based on the code @am11 shared above and made the interpreter behave like the JIT.

@janvorli
Copy link
Member Author

/ba-g The windows arm64 testing is down

@janvorli janvorli merged commit 65ee8fb into dotnet:main Oct 31, 2025
95 of 103 checks passed
@janvorli janvorli deleted the implement-interpreter-cee-jmp branch October 31, 2025 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants